Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(event): Ability to create and sign events for use with js-ceramic #3

Merged
merged 1 commit into from
May 3, 2023

Conversation

dbcfd
Copy link
Contributor

@dbcfd dbcfd commented Mar 14, 2023

No description provided.

@oed
Copy link
Member

oed commented Mar 24, 2023

What does "document" mean in this context?

  • Looks like you've mainly implemented "commits" so far, can we call them "events"?
    See our spec, we now call them Init, Data, and Time Event
  • Documents as they exist in js-ceramic we wanted to not have part of this code base, instead we should mainly focus on streams and events

@dbcfd
Copy link
Contributor Author

dbcfd commented Mar 24, 2023

What does "document" mean in this context?

* Looks like you've mainly implemented "commits" so far, can we call them "events"?
  See our [spec](https://developers.ceramic.network/protocol/streams/event-log/), we now call them Init, Data, and Time Event

* Documents as they exist in js-ceramic we wanted to not have part of this code base, instead we should mainly focus on streams and events

Yep, this work so far focuses on commits, and then the format for stream id type document (3). I think what you're suggesting is

  • Rename the package to "ceramic-events"
  • Rename Commit to Event. Right now js-ceramic doesn't distinguish between an init event and a data event, but rather a deterministic commit (init event w/o data), signed commit (init event w/ data), and signed commit + stream_id (data event). It seems like if we make this implementation match the spec, it won't work with js-ceramic.

@oed
Copy link
Member

oed commented Mar 24, 2023

Yes, I think that's roughly what I said.

It seems like if we make this implementation match the spec, it won't work with js-ceramic.

Not sure I follow this. js-ceramic should definitely be following the spec. cc @AaronGoldman

@dbcfd
Copy link
Contributor Author

dbcfd commented Mar 28, 2023

@oed Reworked PR to match more closely with terms used in spec. As implemented, this can be used with js-ceramic for creating, updating, and reading MIDs.

@dbcfd dbcfd changed the title Feat document feat(event): Ability to create and sign events for use with js-ceramic Mar 29, 2023
Copy link
Collaborator

@nathanielc nathanielc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, I have a few comments/suggestions below.

My biggest question is how we can update the dag-jose crate to be of use here.

event/src/event.rs Show resolved Hide resolved
}

#[derive(Serialize, Deserialize)]
pub struct Jws {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to understand what needs to change in the dag-jose crate in order to be able to use its types here?

I understand this Jws struct also add the signing/verification logic however I do not see the dag-jose crate used anywhere in the code base. As such seems like we should fix that crate to make it amenable to this code as its purpose was to be a building block for Ceramic events.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was really surprised once I got everything implemented, that the events we pass to ceramic don't actually use dag-jose. They just have the data so that ipfs can encode them as dag-jose.

Seems like there's a mismatch there.

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
core/Cargo.toml Outdated Show resolved Hide resolved
core/src/jwk.rs Show resolved Hide resolved
core/src/jwk.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
event/src/args.rs Outdated Show resolved Hide resolved
event/src/lib.rs Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
core/src/lib.rs Outdated Show resolved Hide resolved
core/src/stream_id.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@nathanielc nathanielc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, feel free to merge when its green.

Cargo.toml Outdated Show resolved Hide resolved
@nathanielc nathanielc force-pushed the feat-document branch 3 times, most recently from 32b47bd to b27da17 Compare April 28, 2023 23:21
@nathanielc
Copy link
Collaborator

ok I updated this PR to pass CI. There are two major changes:

Once #32 merged we can merge this PR

event/src/deterministic_event.rs Outdated Show resolved Hide resolved
event/src/event.rs Outdated Show resolved Hide resolved
event/src/event.rs Outdated Show resolved Hide resolved
Comment on lines +12 to +19
pub struct Event {
/// Cid of the data for the event
pub cid: Cid,
/// The data for the event to be encoded in the block
pub linked_block: DagCborEncoded,
/// JWS signature of the event
pub jws: Jws,
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably don't need to change this now, but events can also contain one cacaoBlock (currently), and possible multiple CACAO blocks in the future.

I wonder if a better structure for an event is just a CAR file with all the relevant IPLD blocks?

If not clear already: both jws and linked_block should be IPLD objects.

Comment on lines 39 to 40
/// Optional commit for this stream
pub commit: Option<Cid>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to introduce "commitIds" yet (or at all)?

There is a painpoint around how commitIds are specified currently because they kind of break the multiformat nature of streamIds.

Me and @AaronGoldman talked about introducing a new way to reference a specific event inside of a streamId. This approach would allow streamIds to always be a real multiformat:
https://github.com/AaronGoldman/CIP/blob/stream-sets-synchronization/CIPs/CIP-124/cip-124.md#eventid-sort-order

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mainly have it there to make sure we can read cid's with commit id's specified. If we don't think we'll have stream id's with commits, I'd gladly remove it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StreamIds with commits are not really used anywhere in the core protocol right now. They are mainly used to show the id of a commit to the user.

…amic

Add event functionality (see https://developers.ceramic.network/protocol/streams/event-log/#events)
which is similar to commit functionality currently found in ceramic.
@nathanielc nathanielc added this pull request to the merge queue May 3, 2023
Merged via the queue into main with commit eff535b May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants